Don’t report missing fields in struct exprs with syntax errors.#153227
Don’t report missing fields in struct exprs with syntax errors.#153227rust-bors[bot] merged 3 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred in need_type_info.rs cc @lcnr Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
The only changes to rustfmt and clippy are necessary added @rustbot label -T-clippy -T-rustfmt |
|
Thanks, these types of fixes are exactly what I was talking about! |
|
Size increase of not uncommonly used AST & HIR nodes. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Don’t report missing fields in struct exprs with syntax errors.
There was a problem hiding this comment.
Alternatively to all of this we could 'just' make the parser recovery smarter and try to recover $path { $ident: $expr $ident: $expr } (and similar) as $path { $ident: $expr, $ident: $expr } instead of $path { $ident: /*error*/ } which is what we currently seem to do.
There was a problem hiding this comment.
That helps with the comma being missing, but not with other parse errors. For example, suppose the field name is missing:
struct SomeStruct {
foo: u16,
bar: bool,
}
fn make_foo() -> u16 { 0xF00 }
fn example() -> SomeStruct {
SomeStruct {
/* foo: */ make_foo(),
bar: true,
}
}The parser can’t reasonably recover into knowing that a foo field should be present. It could recover into something like ..<ExprKind::Err> to make the expression valid without foo, but that could provoke further errors itself (e.g. private/non-exhaustive fields).
Regarding the concern over the AST being larger, what if ast::StructRest/hir::StructTailExpr was given a fourth variant to mark the parse error recovery? That would take up no additional memory, and I think it would work out fairly cleanly, since the missing field error it is meant to suppress can only happen if the struct does not have any .., and it acts a little bit like recovering into there being an ... In fact, now that I’ve written it out, I like this idea better than having an independent error-flag field. What do you think?
There was a problem hiding this comment.
Given the perf-regression, I’ve gone ahead and pushed a revised version with the above implementation strategy.
Note: I recommend reviewing commit by commit, and with indentation changes ignored.
There was a problem hiding this comment.
That helps with the comma being missing, but not with other parse errors. For example, suppose the field name is missing
Fair 👍
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (4e1e63f): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (primary 2.6%, secondary -4.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 479.975s -> 483.021s (0.63%) |
This refactor will make an upcoming change smaller, and also adds exhaustive matching.
This adds a variant `NoneWithError` to AST and HIR representations of the “rest” or “tail”, which is currently always treated identically to the `None` variant.
c6432b7 to
83cdfcb
Compare
83cdfcb to
02f6b3a
Compare
|
r? fmease |
|
r? estebank |
@bors r=estebank rollup- |
…ebank Don’t report missing fields in struct exprs with syntax errors. @Noratrieb [told me](https://internals.rust-lang.org/t/custom-cargo-command-to-show-only-errors-avoid-setting-rustflags-every-time/24032/7?u=kpreid) that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently. Specifically, if the user writes a struct literal with a syntax error, such as ```rust StructName { foo: 1 bar: 2 } ``` the compiler will no longer report that the field `bar` is missing in addition to the syntax error. This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here. ~~The part I’m least happy with is the blast radius of adding another field to `hir::ExprKind::Struct`, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changing `hir::ExprKind::Struct` to a nested struct, the same way it is in `ast::ExprKind`.)~~ The additional information is now stored as an additional variant of `ast::StructRest` / `hir::StructTailExpr`. **Note to reviewers:** I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.
…ebank Don’t report missing fields in struct exprs with syntax errors. @Noratrieb [told me](https://internals.rust-lang.org/t/custom-cargo-command-to-show-only-errors-avoid-setting-rustflags-every-time/24032/7?u=kpreid) that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently. Specifically, if the user writes a struct literal with a syntax error, such as ```rust StructName { foo: 1 bar: 2 } ``` the compiler will no longer report that the field `bar` is missing in addition to the syntax error. This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here. ~~The part I’m least happy with is the blast radius of adding another field to `hir::ExprKind::Struct`, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changing `hir::ExprKind::Struct` to a nested struct, the same way it is in `ast::ExprKind`.)~~ The additional information is now stored as an additional variant of `ast::StructRest` / `hir::StructTailExpr`. **Note to reviewers:** I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.
…uwer Rollup of 8 pull requests Successful merges: - #151864 (Implement AST -> HIR generics propagation in delegation) - #152941 (prefer actual ABI-controling fields over target.abi when making ABI decisions) - #153227 (Don’t report missing fields in struct exprs with syntax errors.) - #152966 (Migrate 11 tests from tests/ui/issues to specific directories) - #153003 (rustdoc: make `--emit` and `--out-dir` mimic rustc) - #153034 (Remove unhelpful hint from trivial bound errors) - #153221 (Add release notes for 1.94.0) - #153297 (Update the name of the Hermit operating system)
…ebank Don’t report missing fields in struct exprs with syntax errors. @Noratrieb [told me](https://internals.rust-lang.org/t/custom-cargo-command-to-show-only-errors-avoid-setting-rustflags-every-time/24032/7?u=kpreid) that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently. Specifically, if the user writes a struct literal with a syntax error, such as ```rust StructName { foo: 1 bar: 2 } ``` the compiler will no longer report that the field `bar` is missing in addition to the syntax error. This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here. ~~The part I’m least happy with is the blast radius of adding another field to `hir::ExprKind::Struct`, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changing `hir::ExprKind::Struct` to a nested struct, the same way it is in `ast::ExprKind`.)~~ The additional information is now stored as an additional variant of `ast::StructRest` / `hir::StructTailExpr`. **Note to reviewers:** I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.
…uwer Rollup of 12 pull requests Successful merges: - #152941 (prefer actual ABI-controling fields over target.abi when making ABI decisions) - #153227 (Don’t report missing fields in struct exprs with syntax errors.) - #153265 (Clarified doc comments + added tests confirming current behavior for intersperse/intersperse_with) - #152966 (Migrate 11 tests from tests/ui/issues to specific directories) - #153003 (rustdoc: make `--emit` and `--out-dir` mimic rustc) - #153034 (Remove unhelpful hint from trivial bound errors) - #153152 (Migration of LintDiagnostic - part 5) - #153177 (disable the ptr_fragment_in_final test on s390x) - #153221 (Add release notes for 1.94.0) - #153279 (feat: Provide an '.item_kind()' method on ItemEnum) - #153297 (Update the name of the Hermit operating system) - #153309 (Cleanup of c-variadic link test)
Rollup merge of #153227 - kpreid:struct-missing-field, r=estebank Don’t report missing fields in struct exprs with syntax errors. @Noratrieb [told me](https://internals.rust-lang.org/t/custom-cargo-command-to-show-only-errors-avoid-setting-rustflags-every-time/24032/7?u=kpreid) that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently. Specifically, if the user writes a struct literal with a syntax error, such as ```rust StructName { foo: 1 bar: 2 } ``` the compiler will no longer report that the field `bar` is missing in addition to the syntax error. This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here. ~~The part I’m least happy with is the blast radius of adding another field to `hir::ExprKind::Struct`, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changing `hir::ExprKind::Struct` to a nested struct, the same way it is in `ast::ExprKind`.)~~ The additional information is now stored as an additional variant of `ast::StructRest` / `hir::StructTailExpr`. **Note to reviewers:** I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.
…t-syntax-error, r=dingxiangfei2009 fix(thir): Include `NoneWithError` in Enum Struct Tail Assertion ### Summary: Fixes the ICE triggered when a syntax error appears inside an enum variant struct literal. PR rust-lang#153227 added `StructTailExpr::NoneWithError` and updated the `match base` arm in the `AdtKind::Enum` branch of `thir/cx/expr.rs`, but overlooked the `assert!(matches!(...))` guard preceding it, which still only listed `None | DefaultFields(_)`. Closes rust-lang#153390 r? @dingxiangfei2009 cc @matthiaskrgr
Rollup merge of #153394 - TKanX:bugfix/153390-ice-enum-struct-syntax-error, r=dingxiangfei2009 fix(thir): Include `NoneWithError` in Enum Struct Tail Assertion ### Summary: Fixes the ICE triggered when a syntax error appears inside an enum variant struct literal. PR #153227 added `StructTailExpr::NoneWithError` and updated the `match base` arm in the `AdtKind::Enum` branch of `thir/cx/expr.rs`, but overlooked the `assert!(matches!(...))` guard preceding it, which still only listed `None | DefaultFields(_)`. Closes #153390 r? @dingxiangfei2009 cc @matthiaskrgr
…ebank Don’t report missing fields in struct exprs with syntax errors. @Noratrieb [told me](https://internals.rust-lang.org/t/custom-cargo-command-to-show-only-errors-avoid-setting-rustflags-every-time/24032/7?u=kpreid) that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently. Specifically, if the user writes a struct literal with a syntax error, such as ```rust StructName { foo: 1 bar: 2 } ``` the compiler will no longer report that the field `bar` is missing in addition to the syntax error. This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here. ~~The part I’m least happy with is the blast radius of adding another field to `hir::ExprKind::Struct`, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changing `hir::ExprKind::Struct` to a nested struct, the same way it is in `ast::ExprKind`.)~~ The additional information is now stored as an additional variant of `ast::StructRest` / `hir::StructTailExpr`. **Note to reviewers:** I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.
Fix ICEs due to incomplete typechecking on struct literals with syntax errors. Fixes rust-lang#153388. Followup to rust-lang#153227. Today I have learned that when we don’t emit a diagnostic *specifically from typeck*, we need to call `self.infcx.set_tainted_by_errors()` to signal that the type checking is incomplete despite the lack of error. r? fmease
Rollup merge of #153453 - kpreid:fix-153388, r=fmease,estebank Fix ICEs due to incomplete typechecking on struct literals with syntax errors. Fixes #153388. Followup to #153227. Today I have learned that when we don’t emit a diagnostic *specifically from typeck*, we need to call `self.infcx.set_tainted_by_errors()` to signal that the type checking is incomplete despite the lack of error. r? fmease
View all comments
@Noratrieb told me that “it is a bug if this recovery causes follow-up errors that would not be there if the user fixed the first error.” So, here’s a contribution to hide a follow-up error that annoyed me recently.
Specifically, if the user writes a struct literal with a syntax error, such as
the compiler will no longer report that the field
baris missing in addition to the syntax error.This is my first time attempting any change to the parser or AST; please let me know if there is a better way to do what I’ve done here.
The part I’m least happy with is the blast radius of adding another field toThe additional information is now stored as an additional variant ofhir::ExprKind::Struct, but this seems to be in line with the style of the rest of the code. (If this were my own code, I would consider changinghir::ExprKind::Structto a nested struct, the same way it is inast::ExprKind.)ast::StructRest/hir::StructTailExpr.Note to reviewers: I recommend reviewing each commit separately, and in the case of the first one with indentation changes ignored.